Ln/max tip age bug fix #1392
Ln/max tip age bug fix #1392
Conversation
…ocol/core into ln/max-tip-age-bug-fix
…e into ln/max-tip-age-bug-fix
consensus/event_loop.go
Outdated
| @@ -680,6 +681,16 @@ func (fe *fastHotStuffEventLoop) tryConstructVoteQCInCurrentView() *FastHotStuff | |||
| // Fetch the validator list at the tip. | |||
| validatorList := fe.tip.validatorList | |||
|
|
|||
| for _, validator := range validatorList { | |||
There was a problem hiding this comment.
I wrote a function called DomainsToString(). Let's use that here and elsewhere instead of iterating over GetDomains().
There was a problem hiding this comment.
Sorry, you can actually use validator.GetDomainsString(). The DomainsToString() function is useful if you don't have access to the validator object, which happens in some cases.
There was a problem hiding this comment.
here we don't have the ValidatorEntry, we only have the Validator interface, so I'll use DomainsToString()
There was a problem hiding this comment.
also renaming to validatorItem so it doesn't overlap w/ the type of name validator that we use in the consensus package.
consensus/event_loop.go
Outdated
| @@ -757,6 +772,24 @@ func (fe *fastHotStuffEventLoop) tryConstructTimeoutQCInCurrentView() *FastHotSt | |||
| // to aggregate timeouts from the previous one. | |||
| timeoutsByValidator := fe.timeoutsSeenByView[fe.currentView-1] | |||
|
|
|||
| glog.V(2).Infof("FastHotStuffEventLoop.tryConstructTimeoutQCInCurrentView: "+ | |||
There was a problem hiding this comment.
This spew isn't useful but the thing below it is. So delete the spew but keep the thing below it. And just swap out DomainsToString to save some redundancy.
lib/blockchain.go
Outdated
| if bc.checkpointBlockInfo != nil { | ||
| highestView = bc.checkpointBlockInfo.LatestView | ||
| } | ||
| bc.checkpointBlockInfoLock.RUnlock() |
There was a problem hiding this comment.
I think this is fine to keep for now. I have a slight worry that we will get into a scenario where we want to IGNORE the checkpoint validator, but we can fix that in some other way if we ever hit that problem.
But also I would rewrite this using defer in its own function to avoid the manual RLock()/RUnlock() pattern.
func (bc *Blockchain) GetHighestView() uint64 {
bc.checkpointBlockInfoLock.RLock()
defer bc.checkpointBlockInfoLock.RUnlock()
if bc.checkpointBlockInfo != nil { return bc.checkpointBlockInfo.LatestView }
return 0
}
| glog.Error(err) | ||
| return nil, err | ||
| } | ||
| if chainState != SyncStateFullyCurrent && !srv.blockchain.params.IsPoSBlockHeight(tipHeight) { |
There was a problem hiding this comment.
Did we change this to fix the SYNCING_HEADERS error on Diamond? I think it's fine to keep regardless just wondering.
There was a problem hiding this comment.
Yes - this was the fix to allow us to allow transaction relay regardless of sync status.
| @@ -2856,6 +2880,9 @@ func (srv *Server) _handleFastHotStuffConsensusEvent(event *consensus.FastHotStu | |||
| } | |||
|
|
|||
| func (srv *Server) _handleValidatorVote(pp *Peer, msg *MsgDeSoValidatorVote) { | |||
| if msg.GetMsgType() != MsgTypeValidatorVote { | |||
There was a problem hiding this comment.
I think these are good to have for defensiveness so I'd leave them.
| // Viper doesn't work when you have environment variables. This is the | ||
| // suggested workaround: | ||
| // https://github.com/spf13/viper/issues/380 | ||
| func GetStringSliceWorkaround(flagName string) []string { |
There was a problem hiding this comment.
I applied this thing pretty quickly, but I think we need to keep it generally so I'd leave it. If you don't have it then it literally can't process comma-separated values when you pass them as env vars.
lib/network_manager.go
Outdated
| @@ -190,8 +190,10 @@ func (nm *NetworkManager) startValidatorConnector() { | |||
| nm.exitGroup.Done() | |||
| return | |||
| case <-time.After(nm.peerConnectionRefreshInterval): | |||
| glog.V(2).Infof("NetworkManager.startValidatorConnector: Refreshing validator indices...") | |||
There was a problem hiding this comment.
Sorry I should have been more clear. Let's also delete these glogs because they're too verbose. They just print every second.
lib/network_manager.go
Outdated
| @@ -404,6 +410,11 @@ func (nm *NetworkManager) processOutboundConnection(conn Connection) (*RemoteNod | |||
| return nil, fmt.Errorf("NetworkManager.handleOutboundConnection: Connection is not an outboundConnection") | |||
| } | |||
|
|
|||
| if !addrmgr.IsRoutable(oc.address) { | |||
There was a problem hiding this comment.
I thought we were going to delete this. Wasn't it commented out in the last revision? If we have it it could mess up nodes running within datacenters.
lib/network_manager.go
Outdated
| @@ -209,9 +209,11 @@ func (nm *NetworkManager) startNonValidatorConnector() { | |||
| nm.exitGroup.Done() | |||
| return | |||
| case <-time.After(nm.peerConnectionRefreshInterval): | |||
| glog.V(2).Infof("NetworkManager.startNonValidatorConnector: Refreshing NON validator indices...") | |||
There was a problem hiding this comment.
Sorry delete this and the one below it as well. Don't want to have any logging in this select.
No description provided.